Skip to content

Tait must be constrained if in sig #113169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 12, 2024
Merged

Tait must be constrained if in sig #113169

merged 3 commits into from
Jun 12, 2024

Conversation

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 29, 2023 •
edited
Loading

r? @compiler-errors

kind of reverts #62423, but that PR only removed cycles in cases that we want to forbid now anyway.

see https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/lcnr.20oli.20meeting/near/370712246 for related discussion and motivating example

a TAIT showing up in a signature doesn't just mean it can register a hidden type, but that it must.

This is the design the types team decided upon for the following reasons

  • avoids a hypothetical situation where getting smarter in trait solving could cause new cycle errors or new inference errors to show up, where today something is treated as opaque.
  • avoids having the situation where a (minor?) change to a function body causes an error, because its TAIT usage suddenly isn't "opaque enough" anymore.
  • avoids having to explain why in some cases you need to put a Tait into a tiny module together with its defining usages. Now you basically gotta always do this, the moment a Tait is in a signature that isn't in the defining scope.
  • avoids false-cycle errors
  • anything diverging from this pattern needs to either
    • move the TAIT declaration and defining function into their own helper sub module
    • use the attributes we'll provide in the future to explicitly opt in or out of being in the defining scope

fixes #117861

reverts #125362 cc @Nilstrieb @joboet

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 29, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

oli-obk
oli-obk commented Jun 29, 2023
View reviewed changes
tests/ui/type-alias-impl-trait/issue-63355.rs Outdated
@@ -1,5 +1,4 @@
#![feature(type_alias_impl_trait)]
// check-pass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only test for which there was no workaround after this change.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jul 5, 2023

☔ The latest upstream changes (presumably #113370) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk closed this Aug 3, 2023
@apiraino apiraino removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2023
@oli-obk oli-obk reopened this Oct 24, 2023
@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch 2 times, most recently from b6612f4 to 62d2ddd Compare October 24, 2023 13:12
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from 62d2ddd to 64f3f5b Compare October 24, 2023 13:36
oli-obk
oli-obk commented Oct 24, 2023
View reviewed changes
tests/ui/type-alias-impl-trait/type-alias-impl-trait-fns.rs Outdated
Comment on lines 1 to 2
// check-pass

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR "reopens" #61863

the test doesn't really add too much new test coverage compared with other tests we already have.

@bors
Copy link
Collaborator

bors commented Oct 25, 2023

☔ The latest upstream changes (presumably #117076) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 25, 2023
@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from 64f3f5b to fd06ad3 Compare October 25, 2023 13:04
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from fd06ad3 to d9b265f Compare October 25, 2023 13:45
@oli-obk oli-obk mentioned this pull request Oct 25, 2023
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Oct 25, 2023

☔ The latest upstream changes (presumably #117172) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from f0a4804 to 1d5ea10 Compare October 26, 2023 06:56
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 30, 2023

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 30, 2023
@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from a54797c to 85f2eca Compare June 12, 2024 08:54
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 12, 2024

@bors r=lcnr

@bors
Copy link
Collaborator

bors commented Jun 12, 2024

📌 Commit 85f2eca has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 12, 2024
@bors
Copy link
Collaborator

bors commented Jun 12, 2024

⌛ Testing commit 85f2eca with merge 02c7a59...

@bors
Copy link
Collaborator

bors commented Jun 12, 2024

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 02c7a59 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 12, 2024
@bors bors merged commit 02c7a59 into rust-lang:master Jun 12, 2024
@rustbot rustbot added this to the 1.81.0 milestone Jun 12, 2024
@bors bors mentioned this pull request Jun 12, 2024
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (02c7a59): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.3% [-0.4%, -0.3%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.0% [-0.4%, 0.7%] 3

Max RSS (memory usage)

Results (primary 0.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 5.2% [2.3%, 8.1%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -4.3% [-5.9%, -2.7%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.5% [-5.9%, 8.1%] 4

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.5%] 8
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.1% [-0.1%, -0.0%] 5
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.0% [-0.1%, 0.5%] 13

Bootstrap: 675.824s -> 676.122s (0.04%)
Artifact size: 320.07 MiB -> 320.07 MiB (0.00%)

@rustbot rustbot added the perf-regression Performance regression. label Jun 12, 2024
@oli-obk oli-obk deleted the tait_must_be_constrained_if_in_sig branch June 12, 2024 12:46
@lqd
Copy link
Member

lqd commented Jun 12, 2024

Probably noise + we need the fix => @rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 12, 2024
@lqd lqd mentioned this pull request Jun 13, 2024
Closed
@Dirbaio Dirbaio mentioned this pull request Jun 18, 2024
Closed
@oli-obk oli-obk mentioned this pull request Jun 18, 2024
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
@GuillaumeGomez
Rollup merge of rust-lang#123782 - oli-obk:equal_tait_args, r=compile…
e5f75c6 
…r-errors

Test that opaque types can't have themselves as a hidden type with incompatible lifetimes

fixes rust-lang#122876

This PR used to add extra logic to prevent those cases, but after rust-lang#113169 this is implicitly rejected, because such usages are not defining.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
@GuillaumeGomez
Rollup merge of rust-lang#123782 - oli-obk:equal_tait_args, r=compile…
3b7bb08 
…r-errors

Test that opaque types can't have themselves as a hidden type with incompatible lifetimes

fixes rust-lang#122876

This PR used to add extra logic to prevent those cases, but after rust-lang#113169 this is implicitly rejected, because such usages are not defining.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 19, 2024
@jieyouxu
Rollup merge of rust-lang#123782 - oli-obk:equal_tait_args, r=compile…
e7eb75b 
…r-errors

Test that opaque types can't have themselves as a hidden type with incompatible lifetimes

fixes rust-lang#122876

This PR used to add extra logic to prevent those cases, but after rust-lang#113169 this is implicitly rejected, because such usages are not defining.
fmease added a commit to fmease/rust that referenced this pull request Jun 19, 2024
@fmease
Rollup merge of rust-lang#123782 - oli-obk:equal_tait_args, r=compile…
f9c6e86 
…r-errors

Test that opaque types can't have themselves as a hidden type with incompatible lifetimes

fixes rust-lang#122876

This PR used to add extra logic to prevent those cases, but after rust-lang#113169 this is implicitly rejected, because such usages are not defining.
fmease added a commit to fmease/rust that referenced this pull request Jun 19, 2024
@fmease
Rollup merge of rust-lang#123782 - oli-obk:equal_tait_args, r=compile…
f03bd96 
…r-errors

Test that opaque types can't have themselves as a hidden type with incompatible lifetimes

fixes rust-lang#122876

This PR used to add extra logic to prevent those cases, but after rust-lang#113169 this is implicitly rejected, because such usages are not defining.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
@rust-timer
Unrolled build for rust-lang#123782
5c41bc1 
Rollup merge of rust-lang#123782 - oli-obk:equal_tait_args, r=compiler-errors

Test that opaque types can't have themselves as a hidden type with incompatible lifetimes

fixes rust-lang#122876

This PR used to add extra logic to prevent those cases, but after rust-lang#113169 this is implicitly rejected, because such usages are not defining.
xxchan added a commit to risingwavelabs/risingwave that referenced this pull request Jun 20, 2024
@xxchan
downgrade due to TAIT
1b95f15 
rust-lang/rust#113169

Signed-off-by: xxchan <[email protected]>
@xxchan xxchan mentioned this pull request Jul 11, 2024
Merged
9 tasks
@rrauch rrauch mentioned this pull request Aug 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@compiler-errors compiler-errors

Labels

A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

type alias impl trait stabilization
Status: Done

Milestone

1.81.0

Development

Successfully merging this pull request may close these issues.

TAIT decision on "may define implies must define"